Fixes #766, Support in memory filtering after ODataQueryOptions.ApplyTo #824

xuzhg · 2023-01-26T00:11:29Z

Fixes #766.

If we have $select and $expand in the request query, the result is collection of ISelectExpandWrapper.

It's not easy for customers to do further. So Add 'Cast' Extension methods on IQueryable and Object.

Basic usage in the controller/action:

IQueryable query = options.ApplyTo(....);
IEnumerable<Customer> results = query.OCast<Customer>(query); // the new extension methods on IQueryable.

foreach (var a in results)
{
    // a is a partial object of Customer if we have $select and $expand.
}

Thanks.

…To()

xuzhg · 2023-01-26T00:15:23Z

@julealgon Please take a look and share your thoughts.

julealgon · 2023-01-27T13:51:12Z

src/Microsoft.AspNetCore.OData/Formatter/Value/TypedEdmStructuredObject.cs

+            PropertyInfo propertyInfo = null;
+            var properties = type.GetProperties().Where(p => p.Name == propertyName).ToArray();
+            if (properties.Length <= 0)
+            {
+                propertyInfo = null;
+            }
+            else if (properties.Length == 1)
+            {
+                propertyInfo = properties[0];
+            }
+            else
+            {
+                // resolve 'new' modifier
+                propertyInfo = properties.FirstOrDefault(p => p.DeclaringType == type);
+                if (propertyInfo == null)
+                {
+                    propertyInfo = properties[0];
+                }
+            }


This looks very convoluted to me. Wouldn't this result in the exact same behavior?

Suggested change

PropertyInfo propertyInfo = null;

var properties = type.GetProperties().Where(p => p.Name == propertyName).ToArray();

if (properties.Length <= 0)

{

propertyInfo = null;

}

else if (properties.Length == 1)

{

propertyInfo = properties[0];

}

else

{

// resolve 'new' modifier

propertyInfo = properties.FirstOrDefault(p => p.DeclaringType == type);

if (propertyInfo == null)

{

propertyInfo = properties[0];

}

}

var propertyInfo = type

.GetProperties()

.FirstOrDefault(p => p.Name == propertyName && p.DeclaringType == type);

Also, I wonder if it isn't possible to use one of the GetProperty overloads with BindingFlags to get the same behavior in an even cleaner way without having to go through every single property on the type.

Did you check that possibility?

I did use BindingFlags, but it seems it can't work. (Maybe what I tried is not enough).

julealgon · 2023-01-27T13:52:52Z

src/Microsoft.AspNetCore.OData/Query/IQueryableODataExtension.cs

+    /// <summary>
+    /// The cast options.
+    /// </summary>
+    public class ODataCastOptions
+    {
+        /// <summary>
+        /// Gets/sets the map provider.
+        /// </summary>
+        public Func<IEdmModel, IEdmStructuredType, IPropertyMapper> MapProvider { get; set; }
+    }


Can we move each class to its own file?

julealgon · 2023-01-27T13:54:07Z

src/Microsoft.AspNetCore.OData/Query/IQueryableODataExtension.cs

+    /// <summary>
+    /// Provides a set of static methods for querying data structures that implement <see cref="IQueryable"/>
+    /// </summary>
+    public static class IQueryableODataExtension


Usually, extension classes on interfaces do not have the I in the name:

Suggested change

public static class IQueryableODataExtension

public static class QueryableODataExtension

julealgon · 2023-01-27T13:55:17Z

src/Microsoft.AspNetCore.OData/Query/IQueryableODataExtension.cs

+        /// <param name="options">The cast options.</param>
+        /// <returns>The converted object if it's OData object. Otherwise, return same source or the default.</returns>
+        [SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "<Pending>")]
+        public static TResult OCast<TResult>(this object source, ODataCastOptions options = null)


I think "OCast" is a really bad name that is not intuitive/semantic at all...

I don't have an immediate suggestion, but I'd strongly suggest reconsidering this name.

julealgon · 2023-01-27T13:56:06Z

src/Microsoft.AspNetCore.OData/Query/IQueryableODataExtension.cs

+        /// <param name="source">The source.</param>
+        /// <param name="options">The cast options.</param>
+        /// <returns>The converted object if it's OData object. Otherwise, return same source or the default.</returns>
+        [SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "<Pending>")]


If you really want to suppress this, we should add the justification there.

julealgon · 2023-01-27T15:14:02Z

src/Microsoft.AspNetCore.OData/Query/IQueryableODataExtension.cs

+                    IEnumerable collectionPropertyValue = propertyValue as IEnumerable;
+                    foreach (var item in collectionPropertyValue)


This is a mistake. as should not be used when the semantic is "the value must be of the type", since it can return null and this will cause a NullReferenceExeception in the foreach.

Suggested change

IEnumerable collectionPropertyValue = propertyValue as IEnumerable;

foreach (var item in collectionPropertyValue)

IEnumerable collectionPropertyValue = (IEnumerable)propertyValue;

foreach (var item in collectionPropertyValue)

julealgon · 2023-01-27T15:16:03Z

src/Microsoft.AspNetCore.OData/Query/IQueryableODataExtension.cs

+            if (typeof(ISelectExpandWrapper).IsAssignableFrom(valueType))
+            {
+                SelectExpandWrapper subWrapper = value as SelectExpandWrapper;


This looks dangerous to me. We are checking for a match with the interface ISelectExpandWrapper, but then casting to the concrete type.

It could be possible that the value implements the interface but is not the concrete SelectExpandWrapper, which would cause this to fail.

julealgon · 2023-01-27T15:17:18Z

src/Microsoft.AspNetCore.OData/Query/IQueryableODataExtension.cs

+            PropertyInfo propertyInfo = null;
+            var properties = type.GetProperties().Where(p => p.Name == propertyName).ToArray();
+            if (properties.Length <= 0)
+            {
+                propertyInfo = null;
+            }
+            else if (properties.Length == 1)
+            {
+                propertyInfo = properties[0];
+            }
+            else
+            {
+                // resolve 'new' modifier
+                propertyInfo = properties.FirstOrDefault(p => p.DeclaringType == type);


Duplicated code.

julealgon · 2023-01-27T15:18:22Z

src/Microsoft.AspNetCore.OData/Query/IQueryableODataExtension.cs

+            }
+        }
+
+        private static PropertyInfo GetPropertyInfo(Type type, string propertyName)


Looks like this could be extracted to its own separate extension method. To me this is such a generic concern that doesn't need to be inside the OData-specific extension class.

julealgon · 2023-01-27T15:22:02Z

test/Microsoft.AspNetCore.OData.Tests/Query/IQueryableODataExtensionTests.cs

+        [Fact]
+        public void OCast_Works_ForSingleObject()
+        {
+            // Arrange & Act & Assert
+            object source = null;
+            Assert.Null(source.OCast<object>());
+
+            // Arrange & Act & Assert
+            QueryCustomer customer = new QueryCustomer
+            {
+                Name = "A"
+            };
+
+            Assert.Null(customer.OCast<string>());
+
+            var expectCustomer = customer.OCast<QueryCustomer>();
+            Assert.Same(customer, expectCustomer);
+        }


I'd strongly recommend breaking this into 3 separate tests either manually, or using data-driven tests.

bogdan-patraucean · 2023-10-03T11:56:17Z

We need this. Any updates??

mikepizzo · 2023-10-03T16:39:33Z

We had a discussion on this today -- what is the expected behavior if there is a $select applied to the query on which OCast() is called? should we ignore the $select and set all of the properties, or should we only set the properties that are specified in $select and leave it to the developer to somehow understand which properties have been set and which have not?

bogdan-patraucean · 2023-10-03T17:19:14Z

Awesome! I would expect the select to work. I can't use the select clause with IQueryable so I have to add the needed properties in my endpoint using the Select method.

From my understanding, this PR would fix the issue.

julealgon · 2023-10-03T19:47:02Z

or should we only set the properties that are specified in $select

Wouldn't that clash with required properties? What if the user didn't select some properties that are marked as required in the original model? If you then try to recreate the same object but only set the selected properties, this will fail.

corranrogue9 · 2024-08-06T16:52:11Z

src/Microsoft.AspNetCore.OData/Query/IQueryableODataExtension.cs

+        public static IEnumerable<TResult> OCast<TResult>(this IQueryable source, ODataCastOptions options = null)
+        {
+            if (source is null)
+            {


this won't throw until enumeration; use .select instead

corranrogue9 · 2024-08-06T16:55:06Z

src/Microsoft.AspNetCore.OData/Query/IQueryableODataExtension.cs

+        /// <returns>Contains each element of the source sequence converted to the specified type.</returns>
+        [SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "<Pending>")]
+        public static IEnumerable<TResult> OCast<TResult>(this IQueryable source, ODataCastOptions options = null)
+        {


shouldn't we cast to iqueryable and then do the operations? in cases where the queryable is not a selectexpand, what's written here will result in executing the query on the server just to find out that the method should have been a no-op

Fixes #766, Support in memory filtering after ODataQueryOptions.Apply…

ba1cd44

…To()

xuzhg requested review from KenitoInc, ElizabethOkerio, gathogojr, habbes, lisicase and mikepizzo January 26, 2023 00:11

julealgon reviewed Jan 27, 2023

View reviewed changes

xuzhg mentioned this pull request Mar 21, 2023

How convert result from ApplyTo to IQueryable<T> #865

Closed

AndreaCuneo mentioned this pull request Apr 7, 2023

MissingMethodException with OData v8.1.0 dotnet/aspnet-api-versioning#980

Closed

1 task

xuzhg mentioned this pull request Aug 6, 2024

$expand not working with ODataQueryOptions in the backend #1290

Open

corranrogue9 reviewed Aug 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #766, Support in memory filtering after ODataQueryOptions.ApplyTo #824

Fixes #766, Support in memory filtering after ODataQueryOptions.ApplyTo #824

xuzhg commented Jan 26, 2023

xuzhg commented Jan 26, 2023

julealgon Jan 27, 2023

xuzhg Jan 31, 2023

julealgon Jan 27, 2023

julealgon Jan 27, 2023

julealgon Jan 27, 2023

julealgon Jan 27, 2023

julealgon Jan 27, 2023

julealgon Jan 27, 2023

julealgon Jan 27, 2023

julealgon Jan 27, 2023

julealgon Jan 27, 2023

bogdan-patraucean commented Oct 3, 2023

mikepizzo commented Oct 3, 2023

bogdan-patraucean commented Oct 3, 2023 •

edited

Loading

julealgon commented Oct 3, 2023

corranrogue9 Aug 6, 2024

corranrogue9 Aug 6, 2024

	public static class IQueryableODataExtension
	public static class QueryableODataExtension

		IEnumerable collectionPropertyValue = propertyValue as IEnumerable;
		foreach (var item in collectionPropertyValue)

Fixes #766, Support in memory filtering after ODataQueryOptions.ApplyTo #824

Are you sure you want to change the base?

Fixes #766, Support in memory filtering after ODataQueryOptions.ApplyTo #824

Conversation

xuzhg commented Jan 26, 2023

xuzhg commented Jan 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdan-patraucean commented Oct 3, 2023

mikepizzo commented Oct 3, 2023

bogdan-patraucean commented Oct 3, 2023 • edited Loading

julealgon commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdan-patraucean commented Oct 3, 2023 •

edited

Loading